feat: Implement true_disp calculation in ctapipe-process + test#2951
feat: Implement true_disp calculation in ctapipe-process + test#2951koopatroopa787 wants to merge 26 commits into
Conversation
|
hi @koopatroopa787 , nice timing 🙂 I was actually working on something similar, but you got there first! I just wanted to share one small thought: since the same true_disp calculation logic already exists in train_disp_reconstructor, it might be worth considering moving the shared computation into a small utility function (e.g. under ctapipe.reco) so both places can call the same implementation. That could help avoid duplication and keep the definitions consistent in the future. |
| value = get_dataset_path(value.partition("dataset://")[2]) | ||
| elif url.scheme in ("", "file"): | ||
| value = pathlib.Path(url.netloc, url.path) | ||
| elif os.name == "nt" and len(url.scheme) == 1: |
There was a problem hiding this comment.
Please remove this unrelated change from this pull request.
There was a problem hiding this comment.
Hii, On Windows, I found that traitlets (or the ctapipe wrapper) struggles to validate absolute paths. It misinterprets the drive letter (e.g., C:) as a URL scheme (like http:), which causes validation to fail. I have reverted the fix for now to keep this PR strictly focused on the true_disp implementation, but I wanted to mention it in case it's an issue the team would like addressed in a separate, dedicated PR later.
There was a problem hiding this comment.
We at the moment explicitly do not support and do not test on windows.
| true_parameters.concentration, | ||
| true_parameters.morphology, | ||
| true_parameters.intensity_statistics, | ||
| true_parameters.true_disp, |
There was a problem hiding this comment.
disp is not an image parameter and does not belong in this group.
| if self.should_compute_dl1: | ||
| self.process_images(event) | ||
|
|
||
| if event.simulation is not None: |
There was a problem hiding this comment.
Please do not put inline computations inside of the process tool. The CLI is a high-level wrapper around ctapipe functionality and should only contain minimal code, certainly not the computations itself.
| Container for true disp parameters | ||
| """ | ||
| default_prefix = "true_disp" | ||
| norm = Field(nan * u.deg, "True disp norm", unit=u.deg) |
There was a problem hiding this comment.
We store the predicted disp as a single column, not in separate norm and sign columns.
The true disp should be the same.
| default_factory=CoreParametersContainer, | ||
| description="Image direction in the Tilted/Ground Frame", | ||
| ) | ||
| true_disp = Field( |
There was a problem hiding this comment.
This is not really an image parameter, so should not be part of this data structure.
It rather should be its own subfiled of the SimulatedCameraContainer
There was a problem hiding this comment.
It can also directly use the DispContainer.
|
Hey @yaochengchen and @maxnoe , thanks for the help with this! I'm still getting the hang of the architecture here, so I really appreciate the pointers. I’ve just pushed the fixes we talked about: Moved the true_disp math out of the tool and into a utility in ctapipe.reco.preprocessing. Shifted the field to SimulatedCameraContainer and kept it as a single column to match the predicted format. Updated the tests so they’re all squared away with the new structure. Also, sorry for the Windows-specific quirks earlier! I'll make sure to test things on a Linux VM next time so I don't run into those pathing issues again. Let me know if this looks better! |
| @u.quantity_input( | ||
| fov_lon=u.deg, fov_lat=u.deg, hillas_psi=u.rad, hillas_lon=u.deg, hillas_lat=u.deg | ||
| ) | ||
| def calculate_true_disp(fov_lon, fov_lat, hillas_psi, hillas_lon, hillas_lat): |
There was a problem hiding this comment.
It's good to add this utility, but it should then also be used in the training.
Note that we have two different conventions for computing true disp and this should be configurable here (see the code in the training tool / the project_disp option)
|
@koopatroopa787 I am seeing files here that simply do not belong and I see you re-introduced a change that we agreed was off-topic here. I have the feeling that you are using an automated AI tool to author these changes. While we do not have a policy against that, please note that
We will close this merge request in case we think we are fighting with a model, not collaborating with a human. |
Hi @maxnoe, sorry about the push. I was moving things over to my Ubuntu server for local testing from my windows and accidentally pushed to the PR. it wasn't supposed to go there. I also get now why the The actual bug is in containers=[
event.index,
true_parameters.hillas,
event.simulation.tel[tel_id].true_disp, # wrong, this is an Angle not a Container
]I was dropping a raw value into that list and that is what broke things. I'll get that sorted before the next revision. Also, I will take a bit more time to fix the error as I am still trying to understand the complete repo structure, and regarding the AI automation, I do use it for code generation but I use my own logic and understanding before pushing it. |
incorrectly checked SIMULATION_TEL_TABLE due to a copy-paste error. Replace with proper DL1 trigger table check and add regression test.
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
4 participants
Description
This PR implements the calculation and storage of true_disp parameters (norm and sign) in
ctapipe-processfor simulated events, resolving #2950.It calculates true_disp using the telescope pointing and the simulated shower direction, providing a ground truth for training machine learning models.
Changes
normandsign) and integrated it into ImageParametersContainer.Quantityobjects were incorrectly treated as Containers.Verification
true_disp_normandtrue_disp_signare correctly calculated and written.pytest.